Toogle switch for boolean settings #90
Conversation
| this.switchTransition = new ParallelTransition(this.switchAnimation, this.fillAnimation); | ||
| this.switchAnimation.setNode(this.switchButton); | ||
| this.fillAnimation.setShape(this.switchBackground); | ||
| getChildren().addAll(this.switchBackground, this.switchButton); |
There was a problem hiding this comment.
Missing "this." statement
| /** | ||
| * Boolean property to keep track of the state. | ||
| */ | ||
| private final SimpleBooleanProperty switchedOn; |
There was a problem hiding this comment.
Consider better variable naming, e.g. sth like "switchStateBooleanProperty"
| this.switchTransition.play(); | ||
| }); | ||
| //Mouse listener. | ||
| setOnMouseClicked(event -> this.switchedOn.set(!this.switchedOn.get())); |
There was a problem hiding this comment.
Missing "this." statement.
| return this.switchedOn; | ||
| } | ||
| /** | ||
| * returns isSwitchedOn to change boolean state to true. |
There was a problem hiding this comment.
Why is sth changed here? Or what does the comment mean?
| import javafx.util.Duration; | ||
|
|
||
| /** | ||
| * A toggle switch to en- and disable features in settings. |
There was a problem hiding this comment.
Can you describe your switch, what it does, etc. a little more here? Like, it's a circle on top of a rectangle, the transition is animated, the state on vs off is shown by coloring the rectangle differently, resizing is not implemented (yet), ...
| * https://youtu.be/maX5ymmQixM?si=v2ULa57-pjCmoQlf, 05/17/2024, 10:33 | ||
| */ | ||
| super(); | ||
| this.switchedOn = new SimpleBooleanProperty(false); |
There was a problem hiding this comment.
Would be nice to turn the initial state into a constructor parameter and implement a separate parameter-less constructor calling this one here with a defined initial state (off/false).
| this.switchButton.setEffect(new DropShadow(5, Color.GRAY)); | ||
| this.switchAnimation = new TranslateTransition(Duration.seconds(0.25)); | ||
| this.switchAnimation.setNode(this.switchButton); | ||
| this.fillAnimation = new FillTransition(Duration.seconds(0.25)); |
There was a problem hiding this comment.
Size parameters like the rectangle size, the circle size, etc., should be configurable via setters (getters are then also needed, of course). Also the colors and transition times. The values you are using here should be defined in class constants and used as defaults. Shadows should be able to be turned off and on.
If we would do this more professionally, we would also define resizing and how the switch behaves for a given min/preferred/max height/width. But this goes beyond our current use case.
But at least, that these behaviours are not implemented should be noted in the class documentation because e.g. the setPrefHeight() method is defined by the extended Control class but your switch behaves weirdly if it is used.
| //Listener | ||
| this.switchedOn.addListener((observable, oldValue, newValue) -> { | ||
| boolean tmpIsOn = newValue.booleanValue(); | ||
| this.switchAnimation.setToX(tmpIsOn ? (44 - 18) : 0); |
There was a problem hiding this comment.
These values refer to the rectangle sizes, right? Then please query them from the rectangle instead of writing them explicitly again.
| tmpBooleanComboBox.setPrefWidth(GuiDefinitions.GUI_TEXT_FIELD_PREF_WIDTH_VALUE); | ||
| tmpBooleanComboBox.setMaxWidth(GuiDefinitions.GUI_SETTINGS_TEXT_FIELD_MAX_WIDTH_VALUE); | ||
| tmpBooleanComboBox.getItems().addAll(Boolean.FALSE, Boolean.TRUE); | ||
| tmpBooleanComboBox.valueProperty().bindBidirectional(tmpSimpleBooleanProperty); | ||
| tmpBooleanComboBox.setTooltip(tmpTooltip); | ||
| //add to gridpane | ||
| aGridPane.add(tmpBooleanComboBox, 1, tmpRowIndex++); | ||
| GridPane.setMargin(tmpBooleanComboBox, new Insets(GuiDefinitions.GUI_INSETS_VALUE)); | ||
| GridPane.setMargin(tmpBooleanComboBox, new Insets(GuiDefinitions.GUI_INSETS_VALUE));*/ |
There was a problem hiding this comment.
The to do comment "implement toggle switch here" and the commented-out code for the old combo box solution can be removed now.
src/main/java/de/unijena/cheminf/mortar/gui/controls/ToggleSwitch.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * Default value for the height of the Background. | ||
| */ | ||
| private static final int RECTANGLE_WIDTH_VALUE = 45; |
There was a problem hiding this comment.
The default values should be public.
| /** | ||
| * Default value for the width of the Background. | ||
| */ | ||
| private static final int RECTANGLE_HEIGHT_VALUE = 18; |
There was a problem hiding this comment.
You mixed up width and height in the comments here and above.
| */ | ||
| private static final int RECTANGLE_POSITION_VALUE = -50; | ||
| /** | ||
| * Default value for the Layout which sets the position of the button on the x-axis. |
There was a problem hiding this comment.
... in "off" position?
| /** | ||
| * Default color for the background when turned on. | ||
| */ | ||
| private static final Color RECTANGLE_COLOR_ON = Color.web("#0099cc"); |
There was a problem hiding this comment.
Could you add in the comment what color that hex value represents?
src/main/java/de/unijena/cheminf/mortar/gui/controls/ToggleSwitch.java
Outdated
Show resolved
Hide resolved
| * | ||
| * @return switch value | ||
| */ | ||
| public boolean getSwitchStateBooleanProperty() { |
There was a problem hiding this comment.
I'd rename this to getSwitchState().
| * | ||
| * @return BooleanProperty | ||
| */ | ||
| public BooleanProperty valueProperty() { |
There was a problem hiding this comment.
I'd rename this to getSwitchStateProperty().
| public BooleanProperty valueProperty() { | ||
| return this.switchStateBooleanProperty; | ||
| } | ||
| //</editor-fold> |
There was a problem hiding this comment.
I think we need more instance variables for some of the parameters of the constructors and also getters and setters.
There was a problem hiding this comment.
Or maybe not, most variables can be queried from the rectangle/switch/animation.
|
|
Draft pull request for Zeynep's work on #66